-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI Tools, CLI for Steps 1-7 #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Carlos, thanks for the PR! I've taken a look and I have a few comments and questions – perhaps we can talk this through tomorrow?
Also, add some docstrings and doctests
…ep-2-b02_make_spectra_gft
So other steps can find input files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me.
I left a comment below about the warnings
Not meant to be used with cli with non-cli scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
src/icesat2_tracks/clitools.py
Outdated
def suppress_stdout(verbose=False): | ||
if verbose: | ||
yield | ||
else: | ||
with open(os.devnull, "w") as devnull: | ||
old_stdout = sys.stdout | ||
sys.stdout = devnull | ||
try: | ||
yield | ||
finally: | ||
sys.stdout = old_stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a function provided in python 3.4+ which does this – please use that instead!
Check out https://stackoverflow.com/a/37989355/5542581:
import contextlib
with contextlib.redirect_stdout(None):
print("will not print")
print("this will print")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, thanks for the suggestion @hollandjg! I adapted the current implementation from a context manager that was available elsewhere in the repo.
It seems like contextlib.redirect_stdout
is somewhat limited for this use case, however, as it doesn't have the flexibility to do conditional suppression. Can you think of a simple way to refactor the currently implemented suppress_stdout
so that it leverages contextlib.redirect_stdout
? I gave it some thought but couldn't come up with anything straightfoward.
Cli step 7 b06 correct
112 cli step 6 b04 define angle
95 cli step 5 b04 anglepy
…com/brown-ccv/icesat2-tracks into 93-cli-step-4-a02c_iowaga_thredds_priorpy
…ds_priorpy feat: cli step 4 a02c iowaga thredds prior
…ovpy feat: cli step 3 -- b03_plot_spectra_ovpy
…ep-2-b02_make_spectra_gft
feat: add cli to step 2 -- B02_make_spectra_gFT.py
To facilitate reviewing #78, we are splitting it into bite-size chunks. This is the first one.
Changes:
Update: Dependencies and CI Runner for Step 1
Add
typer
and others. The CI runner for Step 1 has also been updated to reflect these changes.Feature: CLI Tools for Step 1
CLI tools for the package and the particular cli for Step 1. It includes argument parsing and validation, and improved feedback output.